[CELEBORN-796] Support for globally disable thread-local cache in the shared PooledByteBufAllocator #1716

cfmcgrady · 2023-07-14T08:50:22Z

What changes were proposed in this pull request?

As title

Why are the changes needed?

As title

Does this PR introduce any user-facing change?

Yes, the thread local cache of shared PooledByteBufAllocator can be disabled by setting celeborn.network.memory.allocator.allowCache=false

How was this patch tested?

Pass GA

codecov · 2023-07-14T09:01:46Z

Codecov Report

Merging #1716 (176cfb1) into main (efc9a87) will decrease coverage by 0.17%.
Report is 5 commits behind head on main.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##             main    #1716      +/-   ##
==========================================
- Coverage   46.86%   46.68%   -0.17%     
==========================================
  Files         162      162              
  Lines       10027    10088      +61     
  Branches      923      929       +6     
==========================================
+ Hits         4698     4709      +11     
- Misses       5019     5071      +52     
+ Partials      310      308       -2

Files Changed	Coverage Δ
...pache/celeborn/common/network/util/NettyUtils.java	`37.94% <61.54%> (+8.31%)`	⬆️
...cala/org/apache/celeborn/common/CelebornConf.scala	`87.47% <100.00%> (+0.04%)`	⬆️

... and 12 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pan3793 · 2023-07-14T14:52:15Z

Will disable cache hurt performance and increase cpu usage?

github-actions · 2023-08-04T08:30:36Z

This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days.

pan3793 · 2023-08-07T01:11:35Z

Let's revise this PR, it's beneficial to release the direct memory.

I hit worker memory issues recently, the worker triggers trim action, then the disk buffer become empty but direct memory does not reach the threshold, causing the worker being a PAUSE state forever.

MemoryManager#183 - Direct memory usage: 1536.0 MiB/3.0 GiB, disk buffer size: 0.0 B, sort memory size: 0.0 B, read buffer size: 0.0 B
StorageManager#51 - Trigger org.apache.celeborn.service.deploy.worker.storage.StorageManager trim action
StorageManager#51 - Trigger org.apache.celeborn.service.deploy.worker.storage.StorageManager trim action
...

…Allocator.

pan3793 · 2023-08-07T03:12:49Z

common/src/main/java/org/apache/celeborn/common/network/util/NettyUtils.java

@@ -42,7 +42,8 @@

 /** Utilities for creating various Netty constructs based on whether we're using EPOLL or NIO. */
 public class NettyUtils {
-  private static volatile PooledByteBufAllocator _allocator;
+  private static final PooledByteBufAllocator[] _sharedPooledByteBufAllocator =


how about splitting it into 2 variables?

It's derived from Spark and I prefer to keep it

not a big deal, both are fine.

common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala

pan3793 · 2023-08-07T03:16:34Z

cc @waitinfuture

…scala Co-authored-by: Cheng Pan <[email protected]>

pan3793 · 2023-08-08T05:48:35Z

I think it's good to go as it does not change the default behavior.

Generally disabling cache would hurt performance, any chance to provide a benchmark with cache enabled/disabled? e.g. terasort https://github.com/pan3793/spark-terasort

pan3793 · 2023-08-08T05:53:02Z

BTW, I rolled this patch on a small cluster, it shows helps with memory reduction without
obvious performance dropping.

Before (cache enabled)

After (cache disabled)

pan3793 · 2023-08-08T09:32:45Z

common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala

+    buildConf("celeborn.network.memory.allocator.allowCache")
+      .categories("network")
+      .internal
+      .version("0.4.0")


maybe 0.3.1, if @waitinfuture not oppose to

No opposition:) I think we should merge this PR to 0.3.1

The graph looks great!

cfmcgrady · 2023-08-09T03:10:14Z

the netty memory grows more slowly than before in our production environment after applying this patch.

Before this PR (cache enabled)

After this PR (cache disabled)

waitinfuture · 2023-08-11T13:44:48Z

Thanks! Merging to main/0.3

… shared PooledByteBufAllocator ### What changes were proposed in this pull request? As title ### Why are the changes needed? As title ### Does this PR introduce _any_ user-facing change? Yes, the thread local cache of shared `PooledByteBufAllocator` can be disabled by setting `celeborn.network.memory.allocator.allowCache=false` ### How was this patch tested? Pass GA Closes #1716 from cfmcgrady/allow-cache. Authored-by: Fu Chen <[email protected]> Signed-off-by: zky.zhoukeyong <[email protected]> (cherry picked from commit 6f1bb41) Signed-off-by: zky.zhoukeyong <[email protected]>

…lt to false ### What changes were proposed in this pull request? As title ### Why are the changes needed? I tested 1.1T and 3.3T shuffle, as well as 3T TPCDS with thread cache on and off in the shared PooledByteBufAllocator and find no difference: | Benchmark | Cache On | Cache Off| | -------- | ------- |------- | |1.1T Shuffle| 3.7min/1.9min |3.7min/1.9min| | 3.3T Shuffle| 12min/6.7min |12min/6.2min| | 3T TPCDS | 2645s |2644s| And since the configuration has a big influence to the direct memory usage, see #1716 , it's very necessary to set the default value to false. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Manual test. Closes #1817 from waitinfuture/897. Authored-by: zky.zhoukeyong <[email protected]> Signed-off-by: zky.zhoukeyong <[email protected]>

…lt to false ### What changes were proposed in this pull request? As title ### Why are the changes needed? I tested 1.1T and 3.3T shuffle, as well as 3T TPCDS with thread cache on and off in the shared PooledByteBufAllocator and find no difference: | Benchmark | Cache On | Cache Off| | -------- | ------- |------- | |1.1T Shuffle| 3.7min/1.9min |3.7min/1.9min| | 3.3T Shuffle| 12min/6.7min |12min/6.2min| | 3T TPCDS | 2645s |2644s| And since the configuration has a big influence to the direct memory usage, see #1716 , it's very necessary to set the default value to false. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Manual test. Closes #1817 from waitinfuture/897. Authored-by: zky.zhoukeyong <[email protected]> Signed-off-by: zky.zhoukeyong <[email protected]> (cherry picked from commit 57fdbf0) Signed-off-by: zky.zhoukeyong <[email protected]>

…lt to false ### What changes were proposed in this pull request? As title ### Why are the changes needed? I tested 1.1T and 3.3T shuffle, as well as 3T TPCDS with thread cache on and off in the shared PooledByteBufAllocator and find no difference: | Benchmark | Cache On | Cache Off| | -------- | ------- |------- | |1.1T Shuffle| 3.7min/1.9min |3.7min/1.9min| | 3.3T Shuffle| 12min/6.7min |12min/6.2min| | 3T TPCDS | 2645s |2644s| And since the configuration has a big influence to the direct memory usage, see apache/celeborn#1716 , it's very necessary to set the default value to false. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Manual test. Closes #1817 from waitinfuture/897. Authored-by: zky.zhoukeyong <[email protected]> Signed-off-by: zky.zhoukeyong <[email protected]>

cfmcgrady marked this pull request as draft July 14, 2023 08:50

cfmcgrady changed the title ~~[CELEBORN-796] Support for globally disabled thread-local cache in the shared PooledByteBufAllocator~~ [WIP][CELEBORN-796] Support for globally disabled thread-local cache in the shared PooledByteBufAllocator Jul 14, 2023

github-actions bot added the stale label Aug 4, 2023

cfmcgrady added 2 commits August 7, 2023 10:09

support for globally disabled thread-local cache in the PooledByteBuf…

82637e0

…Allocator.

update

0430039

cfmcgrady force-pushed the allow-cache branch from 457f059 to 0430039 Compare August 7, 2023 02:09

pan3793 reviewed Aug 7, 2023

View reviewed changes

common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala Outdated Show resolved Hide resolved

pan3793 removed the stale label Aug 7, 2023

Update common/src/main/scala/org/apache/celeborn/common/CelebornConf.…

76e7e18

…scala Co-authored-by: Cheng Pan <[email protected]>

cfmcgrady changed the title ~~[WIP][CELEBORN-796] Support for globally disabled thread-local cache in the shared PooledByteBufAllocator~~ [WIP][CELEBORN-796] Support for globally disable thread-local cache in the shared PooledByteBufAllocator Aug 7, 2023

cfmcgrady marked this pull request as ready for review August 7, 2023 07:31

cfmcgrady changed the title ~~[WIP][CELEBORN-796] Support for globally disable thread-local cache in the shared PooledByteBufAllocator~~ [CELEBORN-796] Support for globally disable thread-local cache in the shared PooledByteBufAllocator Aug 7, 2023

pan3793 approved these changes Aug 8, 2023

View reviewed changes

pan3793 reviewed Aug 8, 2023

View reviewed changes

0.4.0 -> 0.3.1

176cfb1

waitinfuture closed this in 6f1bb41 Aug 11, 2023

cfmcgrady deleted the allow-cache branch August 11, 2023 14:09

waitinfuture mentioned this pull request Aug 15, 2023

[CELEBORN-897] Set celeborn.network.memory.allocator.allowCache default to false #1817

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CELEBORN-796] Support for globally disable thread-local cache in the shared PooledByteBufAllocator #1716

[CELEBORN-796] Support for globally disable thread-local cache in the shared PooledByteBufAllocator #1716

cfmcgrady commented Jul 14, 2023 •

edited

Loading

codecov bot commented Jul 14, 2023 •

edited

Loading

pan3793 commented Jul 14, 2023

github-actions bot commented Aug 4, 2023

pan3793 commented Aug 7, 2023

pan3793 Aug 7, 2023

cfmcgrady Aug 7, 2023

pan3793 Aug 8, 2023

pan3793 commented Aug 7, 2023

pan3793 commented Aug 8, 2023

pan3793 commented Aug 8, 2023 •

edited

Loading

pan3793 Aug 8, 2023

waitinfuture Aug 8, 2023 •

edited

Loading

waitinfuture Aug 8, 2023

cfmcgrady commented Aug 9, 2023

waitinfuture commented Aug 11, 2023

[CELEBORN-796] Support for globally disable thread-local cache in the shared PooledByteBufAllocator #1716

[CELEBORN-796] Support for globally disable thread-local cache in the shared PooledByteBufAllocator #1716

Conversation

cfmcgrady commented Jul 14, 2023 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

codecov bot commented Jul 14, 2023 • edited Loading

Codecov Report

pan3793 commented Jul 14, 2023

github-actions bot commented Aug 4, 2023

pan3793 commented Aug 7, 2023

pan3793 Aug 7, 2023

Choose a reason for hiding this comment

cfmcgrady Aug 7, 2023

Choose a reason for hiding this comment

pan3793 Aug 8, 2023

Choose a reason for hiding this comment

pan3793 commented Aug 7, 2023

pan3793 commented Aug 8, 2023

pan3793 commented Aug 8, 2023 • edited Loading

pan3793 Aug 8, 2023

Choose a reason for hiding this comment

waitinfuture Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

waitinfuture Aug 8, 2023

Choose a reason for hiding this comment

cfmcgrady commented Aug 9, 2023

waitinfuture commented Aug 11, 2023

cfmcgrady commented Jul 14, 2023 •

edited

Loading

codecov bot commented Jul 14, 2023 •

edited

Loading

pan3793 commented Aug 8, 2023 •

edited

Loading

waitinfuture Aug 8, 2023 •

edited

Loading